Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DRY RUN mode support that can be used while testing #122

Merged
merged 7 commits into from
Apr 24, 2023

Conversation

mukeshpanchal27
Copy link
Contributor

@mukeshpanchal27 mukeshpanchal27 commented Apr 12, 2023

Description of the Change

Closes #121

Credits

Props @mukeshpanchal27 @joemcgill @felixarntz

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review April 12, 2023 11:09
@mukeshpanchal27 mukeshpanchal27 requested a review from a team as a code owner April 12, 2023 11:09
deploy.sh Outdated
echo "➤ Committing files..."
svn commit -m "Update to version $VERSION from GitHub" --no-auth-cache --non-interactive --username "$SVN_USERNAME" --password "$SVN_PASSWORD"
else
echo "➤ Debug mode: Files not committed."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the point where @joemcgill is looking for some sort of output to view what would have been sent across to SVN

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeffpaul For the unbundling of the Performance Lab plugin, we did a similar thing in which we downloaded the deploy.sh file in our repo and removed this line of code so it would not commit to the SVN repo. Our main intention is that we can check all the processes that the deploy script performs but not commit the files to the SVN repository.

Check WordPress/performance#700 (comment) for more details.

@jeffpaul jeffpaul added this to the Future Release milestone Apr 12, 2023
Copy link
Member

@aaemnnosttv aaemnnosttv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mukeshpanchal27 ! I added a few thoughts but more importantly, this does not appear to work the way it is intended to.

README.md Outdated Show resolved Hide resolved
deploy.sh Outdated Show resolved Hide resolved
deploy.sh Outdated Show resolved Hide resolved
deploy.sh Show resolved Hide resolved
Copy link
Member

@aaemnnosttv aaemnnosttv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mukeshpanchal27 – this is looking better, just a few more thoughts for you 👍

README.md Outdated
@@ -23,6 +23,7 @@ This Action commits the contents of your Git tag to the WordPress.org plugin rep
* `VERSION` - defaults to the tag name; do not recommend setting this except for testing purposes.
* `ASSETS_DIR` - defaults to `.wordpress-org`, customizable for other locations of WordPress.org plugin repository-specific assets that belong in the top-level `assets` directory (the one on the same level as `trunk`).
* `BUILD_DIR` - defaults to `false`. Set this flag to the directory where you build your plugins files into, then the action will copy and deploy files from that directory. Both absolute and relative paths are supported. The relative path if provided will be concatenated with the repository root directory. All files and folders in the build directory will be deployed, `.disignore` or `.gitattributes` will be ignored.
* `DRY_RUN` - defaults to `false`. Set `true` to skip the final commit step and check all the debugging processes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, I think this would actually make more sense as a proper input for the action, like generate-zip. This makes it part of the actual API of the action and less fragile to variance of however someone might use/pass an environment variable. Ultimately it would still be an environment variable in the script, but one that isn't intended to be passed manually.

Then we could follow the same simple conditional style that is used for $INPUT_GENERATE_ZIP

if $INPUT_GENERATE_ZIP; then

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aaemnnosttv, for the feedback. Do we need to add dry-run.yml to this PR so others can see how it will be used?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we would need a new dry-run.yml file. I would suggest updating the action.yml file to set a default for the input

inputs:
  generate-zip:
    description: 'Generate package zip file?'
    default: false
  dry-run:
    description: 'Run the deployment process without committing.'
    default: false

And then set the environment variable based on the input here

      env:
        INPUT_GENERATE_ZIP: ${{ inputs.generate-zip }}
        INPUT_DRY_RUN: ${{ inputs.dry-run }}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @joemcgill 👍

@mukeshpanchal27 this looks like the only part that's left to do here 🙂

deploy.sh Outdated Show resolved Hide resolved
deploy.sh Outdated Show resolved Hide resolved
Copy link

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mukeshpanchal27 This looks great, it's going to be quite handy for testing our debug workflows using this action!

Nothing to add to @aaemnnosttv's feedback from my end.

@jeffpaul jeffpaul modified the milestones: Future Release, 2.2.0 Apr 24, 2023
@jeffpaul jeffpaul changed the title Add DEBUG mode support that can be used while testing Add DRY RUN mode support that can be used while testing Apr 24, 2023
@jeffpaul jeffpaul merged commit 16af012 into 10up:develop Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support a "DRY RUN" mode that can be used while testing
6 participants